-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix fee currency bugs in tx pool #244
Conversation
1a1f5e9
to
656f366
Compare
miner/ordering.go
Outdated
if tx.GasFeeCap.Cmp(baseFee) < 0 { | ||
baseFeeConverted := baseFee | ||
if tx.FeeCurrency != nil { | ||
baseFeeBig, _ := exchange.ConvertCeloToCurrency(rates, tx.FeeCurrency, baseFee.ToBig()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could check for error and return the error here and in L60. Not sure what the consequences would be in that case, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should. If there is an error, baseFeeBig
will be nil
which will cause a panic in Cmp
. The error case is handled sanely everywhere where newTxWithMinerFee
. I'm not totally sure what result we ideally want (stop precessing tx for now vs removing from tx pool) in every case, but the default error handling should be good enough for such a failure case.
I'll push one more commit to handle the error for both ConvertCeloToCurrency
calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the tx wouldn't be added to the list or shifted in the first place. So I guess it would be safe to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. No other changes in the force-push, except for an autosquash to clean up the history now that the review is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
Some code paths are only run on non-local txs. The non-local code path is more important to test, since that is what nearly all users will use in production. Non-local txs need to have a tip >=1, so the zero tip values in tests had to be changed.
Geth nodes can set a minimum tip required to accept txs. In this check, the fee currency not taken into account when fetching the base fee for the tip calculation. We missed this earlier in testing because it was only applied to non-local txs.
Tip and base fee handling was not aware of fee currencies in that function, causing wrong tx handling inside the tx pool.
Before, the gas price was always sufficiently high in all e2e tests to mask certain cases of bad fee currency handling in the tx pool.
87ffd65
to
18af38c
Compare
core/txpool/legacypool/legacypool.go
Outdated
@@ -575,7 +575,7 @@ func (pool *LegacyPool) Pending(filter txpool.PendingFilter) map[common.Address] | |||
// If the miner requests tip enforcement, cap the lists now | |||
if minTipBig != nil && !pool.locals.contains(addr) { | |||
for i, tx := range txs { | |||
if tx.EffectiveGasTipIntCmp(minTipBig, pool.priced.urgent.GetNativeBaseFee()) < 0 { | |||
if tx.EffectiveGasTipIntCmp(minTipBig, pool.priced.urgent.GetBaseFeeIn(tx.FeeCurrency())) < 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hy @karlb I don't understand this. The minBigTip is surely provided in the native currency, so how can comparing it to the tip in the fee currency be reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing in the base fee in the fee currency is necessary to make the tip calculation work. Otherwise, a wrong tip would be calculated due to mixing a native base fee with feecurrency based maxPriorityFee and gasFeeCap.
But you are right that afterwards, a fee currency tip is compared to a native minTipBig and that is wrong. Since minTipBig
is 1 by default and we don't change that anywhere this won't cause any noticable problems, but it is still wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if tx.EffectiveGasTipIntCmp(minTipBig, pool.priced.urgent.GetBaseFeeIn(tx.FeeCurrency())) < 0 { | |
// Here we compare the minBigTip in native currency with the tip calculated in the | |
// fee currency, this is wrong but required in order to make the tip calculation work. | |
// The tip is configured via miner.Config.GasPrice which by default is set to 1 so shouldn't | |
// cause any problems. TODO disable the flag to configure miner.Config.GasPrice ? | |
if tx.EffectiveGasTipIntCmp(minTipBig, pool.priced.urgent.GetBaseFeeIn(tx.FeeCurrency())) < 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification @karlb, should we disable the flag to configure the tip then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do this correctly, people sending a single fee currency wei as maxPriorityFee will not get their txs included if their currency is worth less than Celo. That is totally correct, I just wonder if we might break some txs that wrongly accepted before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 3cc1577
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
const maxFeePerGas = block.baseFeePerGas / 2n; | ||
const request = await walletClient.prepareTransactionRequest({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just for this test right? ie i shouldnt be dividing base Fee by 2 normally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a little unclear from the code but block.baseFeePerGas
is denominated in CELO
and the exchange rate of the FEE_CURRENCY2
is 2
, so in order to calculate the maxFeePerGas
high enough to be accepted we divide the base fee by 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good
This PR fixes two issues where fee currency exchange rates were not applied in the tx pool.
To avoid running in to the same issues again, one additional test is added and e2e tests don't use the local-tx special handling that geth applies by default.